The emphasis prop only accepts 'self' but the documentation mentions 'none' as an option. Consider adding 'none' to the type definition: emphasis?: 'self' | 'none' to match the documented behavior and the story's argTypes.

Fixed! ✨ The emphasis prop now includes both 'self' | 'none' in the type definition (commit b3fdac1).

Consider defining a default value in the type documentation. The comment states 'Default is 'line'' but the type doesn't enforce this. Either add = 'line' in the component signature or explicitly handle the undefined case in the implementation.

The axisPointer parameter now has a default value of 'line' in the component signature (ChartTooltip.tsx line 21), so the behavior matches the documentation 👍

I'm wondering if this should be built into the component or if we should allow the component to accept a className prop, that way it can be passed in if needed. My thought is that this is MMS specific. What do you think?

Correct, and good point! I changed this as you advised to be more flexible. ChartTooltip component now accepts an optional className prop (commit b3fdac1). MMS can now pass in the class darkreader-ignore-inline-style to avoid dark mode issues.

This was originally included but removed before v1 since it made no sense for line charts. Makes sense though for a bar chart of course! If we're going to add this to the x-axis, should we add it to the y-axis as well? It might not add much value but perhaps for consistency it makes sense.

Great call on keeping things consistent! I also discovered with more testing that for the 'category' axis type, it's important to always provide the data prop. otherwise, Apache ECharts won't display the category labels correctly.

To improve consistency and avoid code duplication, I factored out all common axis properties and types into a single shared Axis.types.ts file. Now, both XAxisProps and YAxisProps extend a common AxisProps type, which supports the 'category' type.

Agree with copilot here, I think you missed 'none' in the type here.

Fixed! Added 'none' to the type definition as you and Copilot both suggested (commit b3fdac1) 🎉

nit: not necessary, but it might be cleaner to create ContinuousAxisProps and DiscreteAxisProps, and then set AxisProps = ContinuousAxisProps | DiscreteAxisProps

Especially now that I see we export ContinuousAxisProps anyway

Like your suggestion! 💡 Refactor it to:

type AxisPropsBase = {...}
export type ContinuousAxisProps = {...}
export type DiscreteAxisProps = {...}
export type AxisProps = ContinuousAxisProps | DiscreteAxisProps

nit: don't love this, but not sure if any alternative would be cleaner. This will get clunky if we have multiple non-continuous axis types

I think hoverBehavior (or focusBehavior) would be a clearer prop name here

Sure, done.

One point though, hoverBehavior is definitely more descriptive than emphasis but the only reason I used emphasis is because that's the exact property name in Apache ECharts' API, which makes it easier to map directly to the underlying library.
I'm happy either way, let me know what you think.

One of the principles we had when specing the initial charts work was that we didn't want to tie ourselves too closely to the ECharts library, in case they change things, or we decide to go with a different library, our API is just abstractions of that library

Hmm, I see. I imagine that it'll be a difficult migration as we get more comfortable and tie LG to more features of ECharts. 😁

I think this is why the builds are failing

/home/runner/work/leafygreen-ui/leafygreen-ui/charts/core/src/Series/index.ts (1,20): Module '"./Bar"' has no exported member 'BarProps'.
 ELIFECYCLE  Command failed with exit code 1.
Error:  command finished with error: command (/home/runner/work/leafygreen-ui/leafygreen-ui/charts/core) /home/runner/setup-pnpm/node_modules/.bin/pnpm run tsc exited (1)

Thanks Adam! I noticed it and fixed it on my laptop.
Currently trying to digest Stephen's suggestion (I'm getting myself familiar with how git persists renames and moves when file content changes) and see if I can preserve git history properly.

I haven't used git mv retroactively, so I'm not sure if it's possible. You likely would need to reset to the commit prior to moving the files, run git mv, then re-apply changes. It's not imperative, so feel free to ignore that feedback if time is sensitive

I appreciated your suggestion about using git mv to help preserve git history. Definitely important!

From what I understand, git actually tracks changes as additions and deletions, and doesn't record file moves explicitly. File move detection is performed by git using heuristics (like checking content similarity or hashes), not by storing the movement as part of the commit history.

You made me question my assumptions especially by mentioning the git mv command so I dug in a bit more. It seems that in cases where both file path and content changes, you generally need to commit the move separately from any edits. Otherwise, git's move detection will often miss it if the file changes significantly at the same time.

Since our github workflow enforces squash-and-merge (combine all PR commits into one), any intermediate move commits would get flattened, and we'd lose that history anyway if there were substantial changes to the file contents (which is the case in my PR). So I don't see any way to preserve the move history in this case.

Let me know if I'm misunderstanding something or there's a way to do it.

do we need to keep the BarProps export?

just checked locally, and that seems to be what's breaking the build

Hey Stephen, that's correct.

do we want to add BarHoverBehavior as an export for consumers?